feat: extend 4 REST endpoints + migrate 7 DDP callers#40711
Merged
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: e26ed03 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR migrates several client DDP callers to REST endpoints, extends REST contracts (chat.delete supports fileId; spotlight accepts usernames/type/rid and allows anonymous access; subscriptions.read accepts tmid to mark a thread read; users.setPreferences accepts data.utcOffset), adds readThreadMethod, logs deprecation for legacy DDP methods, and adds E2E tests and changesets. ChangesDDP to REST Migration & Endpoint Enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 17 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40711 +/- ##
===========================================
+ Coverage 70.05% 70.18% +0.12%
===========================================
Files 3355 3355
Lines 129162 129266 +104
Branches 22333 22329 -4
===========================================
+ Hits 90487 90722 +235
+ Misses 35387 35254 -133
- Partials 3288 3290 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Member
Author
|
/jira ARCH-2156 |
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
Comment on lines
+549
to
+559
| const msg = | ||
| 'fileId' in this.bodyParams | ||
| ? await Messages.getMessageByFileId(this.bodyParams.fileId) | ||
| : await Messages.findOneById(this.bodyParams.msgId, { projection: { u: 1, rid: 1 } }); | ||
|
|
||
| if (!msg) { | ||
| return API.v1.failure(`No message found with the id of "${this.bodyParams.msgId}".`); | ||
| const ref = 'fileId' in this.bodyParams ? `the file id of "${this.bodyParams.fileId}"` : `the id of "${this.bodyParams.msgId}"`; | ||
| return API.v1.failure(`No message found with ${ref}.`); | ||
| } | ||
|
|
||
| if (this.bodyParams.roomId !== msg.rid) { | ||
| if ('roomId' in this.bodyParams && this.bodyParams.roomId !== msg.rid) { |
There was a problem hiding this comment.
🟡 Medium: Missing Authorization Check in chat.starMessage/unStarMessage
The chat.starMessage and chat.unStarMessage endpoints allow a user to star or unstar any message by providing its messageId. The implementation retrieves the message from the database and passes it to the starMessage function without verifying if the authenticated user has access to the room where the message resides. An attacker could potentially star/unstar messages in rooms they are not authorized to access.
Trace
graph TD
subgraph SG0 ["apps/meteor/app/2fa/server/code/index.ts"]
checkCodeForUser["checkCodeForUser"]
end
style SG0 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG1 ["apps/meteor/app/api/server/ApiClass.ts"]
generateConnection["Generates a connection object for API requests."]
APIClass.shouldAddRateLimitToRoute["APIClass.shouldAddRateLimitToRoute"]
APIClass.success["Formats a successful API response."]
APIClass.failure["APIClass.failure"]
APIClass.internalError["Formats an internal server error API response."]
APIClass.unauthorized["Formats an unauthorized API response."]
APIClass.forbidden["APIClass.forbidden"]
APIClass.tooManyRequests["APIClass.tooManyRequests"]
APIClass.shouldVerifyRateLimit["APIClass.shouldVerifyRateLimit"]
APIClass.enforceRateLimit["APIClass.enforceRateLimit"]
APIClass.addRateLimiterRuleForRoutes["APIClass.addRateLimiterRuleForRoutes"]
APIClass.processTwoFactor["APIClass.processTwoFactor"]
APIClass.getFullRouteName["APIClass.getFullRouteName"]
APIClass.namedRoutes["APIClass.namedRoutes"]
APIClass.registerTypedRoutesLegacy["APIClass.registerTypedRoutesLegacy"]
APIClass.registerTypedRoutes["APIClass.registerTypedRoutes"]
APIClass.method["Internal helper to register an API method."]
APIClass.get["Registers a GET API route."]
APIClass.post["Registers a POST API route."]
APIClass.addRoute["APIClass.addRoute"]
APIClass.this.parseJsonQuery["APIClass.this.parseJsonQuery"]
APIClass.authenticatedRoute["APIClass.authenticatedRoute"]
APIClass.loginCompatibility["Adapts login credentials for compatibility."]
APIClass.createMeteorInvocation["APIClass.createMeteorInvocation"]
APIClass.applyInvocation["APIClass.applyInvocation"]
end
style SG1 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG2 ["apps/meteor/app/api/server/api.helpers.ts"]
isLegacyPermissionsPayload["isLegacyPermissionsPayload"]
isLightPermissionsPayload["isLightPermissionsPayload"]
isPermissionsPayload["isPermissionsPayload"]
checkPermissionsForInvocation["checkPermissionsForInvocation"]
checkPermissions["checkPermissions"]
parseDeprecation["parseDeprecation"]
end
style SG2 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG3 ["apps/meteor/app/api/server/helpers/getUserInfo.ts"]
isVerifiedEmail["isVerifiedEmail"]
getUserPreferences["getUserPreferences"]
filterOutdatedVersionUpdateBanners["filterOutdatedVersionUpdateBanners"]
getUserCalendar["getUserCalendar"]
getUserInfo["Constructs user information object for API responses."]
end
style SG3 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG4 ["apps/meteor/app/api/server/middlewares/authenticationHono.ts"]
isUserWithUsername["isUserWithUsername"]
authenticationMiddlewareForHono["authenticationMiddlewareForHono"]
end
style SG4 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG5 ["apps/meteor/app/api/server/middlewares/permissions.ts"]
permissionsMiddleware["permissionsMiddleware"]
end
style SG5 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG6 ["apps/meteor/app/api/server/router.ts"]
convertHonoContextToApiActionContext["convertHonoContextToApiActionContext"]
end
style SG6 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG7 ["apps/meteor/app/api/server/v1/chat.ts"]
._Rocket.Chat_apps_meteor_app_api_server_v1_chat.ts{{"Registers and implements REST API endpoints for chat operations such as pinning, updating, starring, following messages, reacting, reporting, deleting, searching, and managing threads."}}
end
style SG7 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG8 ["apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts"]
endpoint["endpoint"]
end
style SG8 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG9 ["apps/meteor/app/utils/lib/getURL.ts"]
getURL_2["_getURL"]
getURLWithoutSettings["getURLWithoutSettings"]
end
style SG9 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG10 ["apps/meteor/app/utils/server/functions/getBaseUserFields.ts"]
getBaseUserFields["getBaseUserFields"]
end
style SG10 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG11 ["apps/meteor/app/utils/server/functions/getDefaultUserFields.ts"]
getDefaultUserFields["Returns the default database fields allowed for user objects."]
end
style SG11 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG12 ["apps/meteor/app/utils/server/getURL.ts"]
getURL["getURL"]
end
style SG12 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG13 ["apps/meteor/app/utils/server/lib/getUserPreference.ts"]
getUserPreference["getUserPreference"]
end
style SG13 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG14 ["apps/meteor/ee/app/api-enterprise/server/middlewares/license.ts"]
license["license"]
end
style SG14 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG15 ["apps/meteor/lib/utils/isObject.ts"]
isObject["isObject"]
end
style SG15 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG16 ["apps/meteor/server/lib/shouldBreakInVersion.ts"]
shouldBreakInVersion["shouldBreakInVersion"]
end
style SG16 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG17 ["ee/apps/account-service/src/lib/utils.ts"]
hashLoginToken["_hashLoginToken"]
end
style SG17 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG18 ["packages/logger/src/index.ts"]
Logger.error["Logger.error"]
end
style SG18 fill:#2a2a2a,stroke:#444,color:#aaa
._Rocket.Chat_apps_meteor_app_api_server_v1_chat.ts --> APIClass.get
._Rocket.Chat_apps_meteor_app_api_server_v1_chat.ts --> APIClass.post
APIClass.get --> APIClass.method
APIClass.post --> getUserInfo
APIClass.post --> generateConnection
APIClass.post --> APIClass.success
APIClass.post --> APIClass.internalError
APIClass.post --> APIClass.unauthorized
APIClass.post --> APIClass.method
APIClass.post --> APIClass.loginCompatibility
APIClass.post --> getDefaultUserFields
APIClass.method --> APIClass.registerTypedRoutes
APIClass.method --> APIClass.addRoute
getUserInfo --> isVerifiedEmail
getUserInfo --> getUserPreferences
getUserInfo --> filterOutdatedVersionUpdateBanners
getUserInfo --> getUserCalendar
getUserInfo --> getURL
APIClass.success --> isObject
APIClass.loginCompatibility --> APIClass.get
getDefaultUserFields --> getBaseUserFields
APIClass.addRoute --> hashLoginToken
APIClass.addRoute --> shouldBreakInVersion
APIClass.addRoute --> generateConnection
APIClass.addRoute --> APIClass.shouldAddRateLimitToRoute
APIClass.addRoute --> APIClass.failure
APIClass.addRoute --> APIClass.unauthorized
APIClass.addRoute --> APIClass.forbidden
APIClass.addRoute --> APIClass.tooManyRequests
APIClass.addRoute --> APIClass.enforceRateLimit
APIClass.addRoute --> APIClass.addRateLimiterRuleForRoutes
APIClass.addRoute --> APIClass.processTwoFactor
APIClass.addRoute --> APIClass.getFullRouteName
APIClass.addRoute --> APIClass.registerTypedRoutesLegacy
APIClass.addRoute --> APIClass.get
APIClass.addRoute --> APIClass.this.parseJsonQuery
APIClass.addRoute --> APIClass.createMeteorInvocation
APIClass.addRoute --> APIClass.applyInvocation
APIClass.addRoute --> authenticationMiddlewareForHono
APIClass.addRoute --> permissionsMiddleware
APIClass.addRoute --> checkPermissions
APIClass.addRoute --> parseDeprecation
APIClass.addRoute --> license
getUserPreferences --> getUserPreference
getURL --> getURLWithoutSettings
APIClass.failure --> isObject
APIClass.enforceRateLimit --> APIClass.shouldVerifyRateLimit
APIClass.addRateLimiterRuleForRoutes --> APIClass.namedRoutes
APIClass.processTwoFactor --> APIClass.get
APIClass.processTwoFactor --> checkCodeForUser
APIClass.registerTypedRoutesLegacy --> APIClass.registerTypedRoutes
APIClass.this.parseJsonQuery --> APIClass.this.parseJsonQuery
authenticationMiddlewareForHono --> APIClass.unauthorized
authenticationMiddlewareForHono --> APIClass.get
authenticationMiddlewareForHono --> APIClass.authenticatedRoute
authenticationMiddlewareForHono --> convertHonoContextToApiActionContext
authenticationMiddlewareForHono --> isUserWithUsername
permissionsMiddleware --> APIClass.internalError
permissionsMiddleware --> APIClass.unauthorized
permissionsMiddleware --> APIClass.forbidden
permissionsMiddleware --> APIClass.get
permissionsMiddleware --> checkPermissionsForInvocation
permissionsMiddleware --> Logger.error
checkPermissions --> isLegacyPermissionsPayload
checkPermissions --> isLightPermissionsPayload
checkPermissions --> isPermissionsPayload
parseDeprecation --> endpoint
getURLWithoutSettings --> getURL_2
APIClass.shouldVerifyRateLimit --> APIClass.get
APIClass.namedRoutes --> APIClass.getFullRouteName
APIClass.authenticatedRoute --> hashLoginToken
APIClass.authenticatedRoute --> APIClass.method
APIClass.authenticatedRoute --> APIClass.get
APIClass.authenticatedRoute --> getDefaultUserFields
Logger.error --> Logger.error
Fix with AI
Fix the following security vulnerability found by Hacktron.
File: apps/meteor/app/api/server/v1/chat.ts
Lines: 549-559
Severity: medium
Vulnerability: Missing Authorization Check in chat.starMessage/unStarMessage
Description:
The `chat.starMessage` and `chat.unStarMessage` endpoints allow a user to star or unstar any message by providing its `messageId`. The implementation retrieves the message from the database and passes it to the `starMessage` function without verifying if the authenticated user has access to the room where the message resides. An attacker could potentially star/unstar messages in rooms they are not authorized to access.
Fix this vulnerability. Only change what's necessary - don't modify unrelated code.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing. Any other reply is saved as a triage note.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/app/lib/server/methods/joinRoom.ts`:
- Line 19: The deprecation log call methodDeprecationLogger.method('joinRoom',
'9.0.0', '/v1/channels.join') is too broad for the multi-room joinRoom method;
update joinRoom so you resolve the room type (e.g., using the existing room
resolution logic inside the joinRoom handler) and only call
methodDeprecationLogger.method with the specific channels route when the
resolved type is a public channel, otherwise either skip the deprecation log or
emit a type-appropriate/generic message; locate the deprecation call inside
joinRoom and gate it behind the room-type check (or replace with a no-op for
non-channel types).
In
`@apps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsx`:
- Line 74: The call to markThreadRead in ThreadChat.tsx is fire-and-forget and
can produce an unhandled rejection; update the invocation (the void
markThreadRead({ rid: room._id, tmid: mainMessage._id }) call) to attach a
rejection handler (e.g. .catch(() => undefined) or log the error) so promise
rejections are swallowed/handled consistently with useThreadMessagesQuery.ts;
locate markThreadRead (from useEndpoint('POST', '/v1/subscriptions.read')) in
ThreadChat and modify the call to add the .catch handler.
In `@apps/meteor/client/views/root/hooks/useLoadMissedMessages.ts`:
- Around line 29-32: In useLoadMissedMessages, besides processing result.updated
with upsertMessage(mapMessageFromApi(...)), also handle result.deleted by
iterating over each deleted entry and removing/hiding that message from local
state (e.g., call Messages.state.delete(deleted._id) or the app’s equivalent
deletion method for each deleted._id) so deleted messages from chat.syncMessages
are not left stale after reconnect; keep the existing subscription lookup and
ensure deletions run before/after updates in a sensible order to avoid race
conditions.
In `@packages/rest-typings/src/v1/misc.ts`:
- Around line 82-87: The parser parseSpotlightType currently returns any object
including arrays; update its success check to only accept plain objects by
rejecting arrays and non-plain prototypes: after JSON.parse(parsed) assert
parsed && typeof parsed === 'object' && !Array.isArray(parsed) &&
(Object.getPrototypeOf(parsed) === Object.prototype || parsed.constructor ===
Object) before returning, otherwise return undefined; keep the function name
parseSpotlightType and the SpotlightType type for clarity.
In `@packages/rest-typings/src/v1/subscriptionsEndpoints.ts`:
- Around line 54-57: The runtime schema for the tmid field allows null (tmid: {
type: 'string', nullable: true }) but the TypeScript contract declares tmid as
optional with type IMessage['_id'] (tmid?: IMessage['_id']) which does not
include null; update the runtime validation to match the TS contract by removing
nullable: true for the tmid schema entries (both occurrences) or change the
TypeScript type to include null if null is intended—ensure the schema and the
IMessage['_id'] usage (tmid) are consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fdc65248-93ac-4e65-aa3d-183950fc83b8
📒 Files selected for processing (23)
apps/meteor/app/api/server/v1/chat.tsapps/meteor/app/api/server/v1/misc.tsapps/meteor/app/api/server/v1/subscriptions.tsapps/meteor/app/lib/server/methods/joinRoom.tsapps/meteor/client/hooks/useJoinRoom.tsapps/meteor/client/lib/chats/data.tsapps/meteor/client/navbar/NavBarSearch/hooks/useSearchItems.tsapps/meteor/client/startup/startup.tsapps/meteor/client/views/room/contextualBar/RoomFiles/hooks/useDeleteFile.tsxapps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsxapps/meteor/client/views/room/contextualBar/Threads/hooks/useThreadMessagesQuery.tsapps/meteor/client/views/room/providers/ComposerPopupProvider.tsxapps/meteor/client/views/root/hooks/useLoadMissedMessages.tsapps/meteor/server/methods/deleteFileMessage.tsapps/meteor/server/methods/loadMissedMessages.tsapps/meteor/server/methods/readThreads.tsapps/meteor/server/methods/saveUserPreferences.tsapps/meteor/server/methods/userSetUtcOffset.tsapps/meteor/server/publications/spotlight.tspackages/rest-typings/src/v1/chat.tspackages/rest-typings/src/v1/misc.tspackages/rest-typings/src/v1/subscriptionsEndpoints.tspackages/rest-typings/src/v1/users/UsersSetPreferenceParamsPOST.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/server/methods/readThreads.tsapps/meteor/client/lib/chats/data.tsapps/meteor/server/methods/userSetUtcOffset.tsapps/meteor/server/methods/deleteFileMessage.tsapps/meteor/client/views/room/contextualBar/Threads/hooks/useThreadMessagesQuery.tsapps/meteor/app/api/server/v1/chat.tsapps/meteor/client/navbar/NavBarSearch/hooks/useSearchItems.tspackages/rest-typings/src/v1/users/UsersSetPreferenceParamsPOST.tsapps/meteor/server/methods/loadMissedMessages.tsapps/meteor/client/views/root/hooks/useLoadMissedMessages.tsapps/meteor/client/startup/startup.tsapps/meteor/app/lib/server/methods/joinRoom.tsapps/meteor/server/publications/spotlight.tsapps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsxapps/meteor/client/views/room/contextualBar/RoomFiles/hooks/useDeleteFile.tsxpackages/rest-typings/src/v1/chat.tsapps/meteor/app/api/server/v1/misc.tsapps/meteor/client/views/room/providers/ComposerPopupProvider.tsxapps/meteor/app/api/server/v1/subscriptions.tsapps/meteor/client/hooks/useJoinRoom.tspackages/rest-typings/src/v1/misc.tspackages/rest-typings/src/v1/subscriptionsEndpoints.tsapps/meteor/server/methods/saveUserPreferences.ts
🧠 Learnings (11)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/server/methods/readThreads.tsapps/meteor/client/lib/chats/data.tsapps/meteor/server/methods/userSetUtcOffset.tsapps/meteor/server/methods/deleteFileMessage.tsapps/meteor/client/views/room/contextualBar/Threads/hooks/useThreadMessagesQuery.tsapps/meteor/app/api/server/v1/chat.tsapps/meteor/client/navbar/NavBarSearch/hooks/useSearchItems.tspackages/rest-typings/src/v1/users/UsersSetPreferenceParamsPOST.tsapps/meteor/server/methods/loadMissedMessages.tsapps/meteor/client/views/root/hooks/useLoadMissedMessages.tsapps/meteor/client/startup/startup.tsapps/meteor/app/lib/server/methods/joinRoom.tsapps/meteor/server/publications/spotlight.tspackages/rest-typings/src/v1/chat.tsapps/meteor/app/api/server/v1/misc.tsapps/meteor/app/api/server/v1/subscriptions.tsapps/meteor/client/hooks/useJoinRoom.tspackages/rest-typings/src/v1/misc.tspackages/rest-typings/src/v1/subscriptionsEndpoints.tsapps/meteor/server/methods/saveUserPreferences.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/server/methods/readThreads.tsapps/meteor/client/lib/chats/data.tsapps/meteor/server/methods/userSetUtcOffset.tsapps/meteor/server/methods/deleteFileMessage.tsapps/meteor/client/views/room/contextualBar/Threads/hooks/useThreadMessagesQuery.tsapps/meteor/app/api/server/v1/chat.tsapps/meteor/client/navbar/NavBarSearch/hooks/useSearchItems.tspackages/rest-typings/src/v1/users/UsersSetPreferenceParamsPOST.tsapps/meteor/server/methods/loadMissedMessages.tsapps/meteor/client/views/root/hooks/useLoadMissedMessages.tsapps/meteor/client/startup/startup.tsapps/meteor/app/lib/server/methods/joinRoom.tsapps/meteor/server/publications/spotlight.tspackages/rest-typings/src/v1/chat.tsapps/meteor/app/api/server/v1/misc.tsapps/meteor/app/api/server/v1/subscriptions.tsapps/meteor/client/hooks/useJoinRoom.tspackages/rest-typings/src/v1/misc.tspackages/rest-typings/src/v1/subscriptionsEndpoints.tsapps/meteor/server/methods/saveUserPreferences.ts
📚 Learning: 2026-03-09T18:39:14.020Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:14.020Z
Learning: When implementing batch processing in server methods, favor a single data pass to collect both the items and any derived fields needed for validation. Use the same dataset for both validation and processing to avoid races between validation and execution, and document the approach in code comments. Apply this pattern to similar Meteor Rocket.Chat server methods under apps/meteor/server/methods to prevent race conditions and ensure consistent batch behavior.
Applied to files:
apps/meteor/server/methods/readThreads.tsapps/meteor/server/methods/userSetUtcOffset.tsapps/meteor/server/methods/deleteFileMessage.tsapps/meteor/server/methods/loadMissedMessages.tsapps/meteor/server/methods/saveUserPreferences.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/server/methods/readThreads.tsapps/meteor/client/lib/chats/data.tsapps/meteor/server/methods/userSetUtcOffset.tsapps/meteor/server/methods/deleteFileMessage.tsapps/meteor/client/views/room/contextualBar/Threads/hooks/useThreadMessagesQuery.tsapps/meteor/app/api/server/v1/chat.tsapps/meteor/client/navbar/NavBarSearch/hooks/useSearchItems.tspackages/rest-typings/src/v1/users/UsersSetPreferenceParamsPOST.tsapps/meteor/server/methods/loadMissedMessages.tsapps/meteor/client/views/root/hooks/useLoadMissedMessages.tsapps/meteor/client/startup/startup.tsapps/meteor/app/lib/server/methods/joinRoom.tsapps/meteor/server/publications/spotlight.tsapps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsxapps/meteor/client/views/room/contextualBar/RoomFiles/hooks/useDeleteFile.tsxpackages/rest-typings/src/v1/chat.tsapps/meteor/app/api/server/v1/misc.tsapps/meteor/client/views/room/providers/ComposerPopupProvider.tsxapps/meteor/app/api/server/v1/subscriptions.tsapps/meteor/client/hooks/useJoinRoom.tspackages/rest-typings/src/v1/misc.tspackages/rest-typings/src/v1/subscriptionsEndpoints.tsapps/meteor/server/methods/saveUserPreferences.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/lib/chats/data.tsapps/meteor/client/views/room/contextualBar/Threads/hooks/useThreadMessagesQuery.tsapps/meteor/client/navbar/NavBarSearch/hooks/useSearchItems.tsapps/meteor/client/views/root/hooks/useLoadMissedMessages.tsapps/meteor/client/startup/startup.tsapps/meteor/client/hooks/useJoinRoom.ts
📚 Learning: 2026-05-11T20:30:35.265Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 40480
File: apps/meteor/client/meteor/startup/accounts.ts:59-61
Timestamp: 2026-05-11T20:30:35.265Z
Learning: In Rocket.Chat’s Meteor client code, when calling `dispatchToastMessage` with `{ type: 'error' }`, pass the raw caught error object as `message` without manual normalization. `dispatchToastMessage` is designed to accept `message: unknown` for error toasts, so avoid converting errors to strings (e.g., `String(error)`) or extracting `error.message` before passing them.
Applied to files:
apps/meteor/client/lib/chats/data.tsapps/meteor/client/views/room/contextualBar/Threads/hooks/useThreadMessagesQuery.tsapps/meteor/client/navbar/NavBarSearch/hooks/useSearchItems.tsapps/meteor/client/views/root/hooks/useLoadMissedMessages.tsapps/meteor/client/startup/startup.tsapps/meteor/client/hooks/useJoinRoom.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/chat.tsapps/meteor/app/api/server/v1/misc.tsapps/meteor/app/api/server/v1/subscriptions.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/chat.tsapps/meteor/app/api/server/v1/misc.tsapps/meteor/app/api/server/v1/subscriptions.ts
📚 Learning: 2026-05-11T23:14:59.316Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 40469
File: packages/rest-typings/src/v1/users.ts:337-337
Timestamp: 2026-05-11T23:14:59.316Z
Learning: In Rocket.Chat REST endpoint typings (e.g., packages/rest-typings/src/v1/users.ts and other rest-typings files), keep the established convention of deriving field types from the domain model (e.g., use IUser indexed access like IUser['statusExpiresAt']) rather than swapping individual fields to serialized primitives (like string) in an ad-hoc way. If a truly different “serialized” representation is needed, perform the refactor consistently across the codebase (not just a single endpoint/field) and ensure all related REST typings stay aligned with the shared serialization types.
Applied to files:
packages/rest-typings/src/v1/users/UsersSetPreferenceParamsPOST.tspackages/rest-typings/src/v1/chat.tspackages/rest-typings/src/v1/misc.tspackages/rest-typings/src/v1/subscriptionsEndpoints.ts
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsxapps/meteor/client/views/room/contextualBar/RoomFiles/hooks/useDeleteFile.tsxapps/meteor/client/views/room/providers/ComposerPopupProvider.tsx
📚 Learning: 2026-04-14T21:10:31.855Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36292
File: apps/meteor/client/hooks/useHasValidLocationHash.ts:7-12
Timestamp: 2026-04-14T21:10:31.855Z
Learning: When reviewing files in apps/meteor/client/hooks/, do not treat JSDoc-style comments on React hooks (especially exported hooks) as a violation of any “avoid code comments in implementation” guideline. It’s acceptable to use JSDoc to document the public API of exported hooks (e.g., parameter/return types, intended usage), as long as it documents behavior/contracts rather than adding narrative implementation comments.
Applied to files:
apps/meteor/client/hooks/useJoinRoom.ts
🔇 Additional comments (21)
apps/meteor/client/hooks/useJoinRoom.ts (1)
1-34: LGTM!apps/meteor/client/lib/chats/data.ts (1)
276-281: LGTM!apps/meteor/client/navbar/NavBarSearch/hooks/useSearchItems.ts (1)
3-3: LGTM!Also applies to: 50-50, 60-64
apps/meteor/client/views/room/providers/ComposerPopupProvider.tsx (1)
6-6: LGTM!Also applies to: 73-73, 142-147, 186-190, 412-412
packages/rest-typings/src/v1/chat.ts (1)
221-230: LGTM!Also applies to: 241-243, 249-249
packages/rest-typings/src/v1/users/UsersSetPreferenceParamsPOST.ts (1)
56-56: LGTM!Also applies to: 272-275
apps/meteor/app/api/server/v1/chat.ts (1)
549-552: LGTM!Also applies to: 555-556, 559-559
apps/meteor/app/api/server/v1/subscriptions.ts (1)
2-2: LGTM!Also applies to: 17-17, 143-143, 151-158
apps/meteor/server/methods/saveUserPreferences.ts (1)
56-56: LGTM!Also applies to: 132-132, 156-160
apps/meteor/app/api/server/v1/misc.ts (1)
392-395: Recheck anonymous access enforcement for thespotlightendpoint
apps/meteor/app/api/server/v1/misc.tssetsauthRequired: false(lines ~392–395, also ~406–412), so unauthenticated requests can reach the endpoint.- On the server side,
apps/meteor/server/publications/spotlight.tsdefines the Meteor methodspotlightwithuserId(/* userId*/) { return true; }, andspotlightMethodforwardsuserId: this.userIdintospotlight.searchUsers(...)/spotlight.searchRooms(...).- In the inspected
apps/meteor/server/publications/spotlight.ts, there are no references toAccounts_AllowAnonymousRead/AllowAnonymousRead(or equivalent) before returning results.Ensure the permission gating (e.g.,
Accounts_AllowAnonymousReador equivalent) is enforced insideSpotlight.searchUsers/Spotlight.searchRooms(or earlier in the call chain); otherwise this change likely exposes searchable data anonymously.apps/meteor/app/lib/server/methods/joinRoom.ts (1)
8-9: LGTM!apps/meteor/server/methods/deleteFileMessage.ts (1)
9-9: LGTM!Also applies to: 20-20
apps/meteor/server/methods/loadMissedMessages.ts (1)
8-8: LGTM!Also applies to: 19-19
apps/meteor/server/methods/readThreads.ts (1)
8-8: LGTM!Also applies to: 22-22
apps/meteor/server/methods/userSetUtcOffset.ts (1)
7-8: LGTM!Also applies to: 18-18
apps/meteor/server/publications/spotlight.ts (1)
5-5: LGTM!Also applies to: 72-72
apps/meteor/client/views/room/contextualBar/RoomFiles/hooks/useDeleteFile.tsx (1)
4-4: LGTM!Also applies to: 11-11, 16-16
apps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsx (1)
5-5: LGTM!Also applies to: 65-65, 83-83
apps/meteor/client/views/room/contextualBar/Threads/hooks/useThreadMessagesQuery.ts (1)
2-2: LGTM!Also applies to: 29-29, 111-111
apps/meteor/client/startup/startup.ts (1)
36-36: LGTM!Also applies to: 60-60
apps/meteor/client/views/root/hooks/useLoadMissedMessages.ts (1)
6-7: LGTM!
|
|
||
| Meteor.methods<ServerMethods>({ | ||
| async joinRoom(rid, code) { | ||
| methodDeprecationLogger.method('joinRoom', '9.0.0', '/v1/channels.join'); |
Contributor
There was a problem hiding this comment.
Deprecation target is too broad for a multi-room joinRoom method.
joinRoom is used for more than public channels, but this log always points to /v1/channels.join. That replacement is not valid for all room types, so this warning can mislead integrators and create migration breakage when DDP is removed. Please gate the deprecation route by resolved room type (or avoid logging for unsupported non-channel types).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/meteor/app/lib/server/methods/joinRoom.ts` at line 19, The deprecation
log call methodDeprecationLogger.method('joinRoom', '9.0.0',
'/v1/channels.join') is too broad for the multi-room joinRoom method; update
joinRoom so you resolve the room type (e.g., using the existing room resolution
logic inside the joinRoom handler) and only call methodDeprecationLogger.method
with the specific channels route when the resolved type is a public channel,
otherwise either skip the deprecation log or emit a type-appropriate/generic
message; locate the deprecation call inside joinRoom and gate it behind the
room-type check (or replace with a no-op for non-channel types).
| } | ||
|
|
||
| readThreads(mainMessage._id); | ||
| void markThreadRead({ rid: room._id, tmid: mainMessage._id }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking `useEndpoint` typing and current `subscriptions.read` call patterns..."
rg -nP --type=ts "function useEndpoint|const useEndpoint|type.*useEndpoint" apps packages -C2 || true
rg -nP --type=ts "subscriptions\.read" apps/meteor/client -C2 || true
rg -nP --type=ts "void\s+markThreadRead\s*\(" apps/meteor/client/views/room/contextualBar/Threads -C2 || trueRepository: RocketChat/Rocket.Chat
Length of output: 6109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' apps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsx | nl -ba | sed -n '50,110p'
echo "---- useThreadMessagesQuery.ts around markThreadRead catch ----"
sed -n '80,150p' apps/meteor/client/views/room/contextualBar/Threads/hooks/useThreadMessagesQuery.ts | nl -ba | sed -n '90,140p'
echo "---- useEndpoint.ts implementation ----"
sed -n '1,120p' packages/ui-contexts/src/hooks/useEndpoint.ts | nl -ba | sed -n '1,120p'Repository: RocketChat/Rocket.Chat
Length of output: 109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' apps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsx | nl -ba | sed -n '50,110p'
echo "---- useThreadMessagesQuery.ts around markThreadRead catch ----"
sed -n '80,160p' apps/meteor/client/views/room/contextualBar/Threads/hooks/useThreadMessagesQuery.ts | nl -ba | sed -n '90,150p'
echo "---- useEndpoint.ts implementation ----"
sed -n '1,160p' packages/ui-contexts/src/hooks/useEndpoint.ts | nl -ba | sed -n '1,160p'Repository: RocketChat/Rocket.Chat
Length of output: 109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' apps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsx | nl -ba | sed -n '50,110p'
echo "---- useThreadMessagesQuery.ts around markThreadRead catch ----"
sed -n '90,140p' apps/meteor/client/views/room/contextualBar/Threads/hooks/useThreadMessagesQuery.ts | nl -ba | sed -n '90,140p'
echo "---- useEndpoint.ts implementation ----"
sed -n '1,200p' packages/ui-contexts/src/hooks/useEndpoint.ts | nl -ba | sed -n '1,200p'Repository: RocketChat/Rocket.Chat
Length of output: 109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,160p' apps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsx | nl -ba | sed -n '55,95p'
echo "---- useThreadMessagesQuery.ts around markThreadRead catch ----"
sed -n '90,130p' apps/meteor/client/views/room/contextualBar/Threads/hooks/useThreadMessagesQuery.ts | nl -ba | sed -n '100,125p'
echo "---- useEndpoint.ts implementation ----"
sed -n '1,120p' packages/ui-contexts/src/hooks/useEndpoint.ts | nl -ba | sed -n '1,120p'Repository: RocketChat/Rocket.Chat
Length of output: 109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- ThreadChat.tsx around markThreadRead ----"
nl -ba apps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsx | sed -n '55,95p'
echo "---- useThreadMessagesQuery.ts around markThreadRead catch ----"
nl -ba apps/meteor/client/views/room/contextualBar/Threads/hooks/useThreadMessagesQuery.ts | sed -n '95,125p'
echo "---- useEndpoint.ts implementation ----"
nl -ba packages/ui-contexts/src/hooks/useEndpoint.ts | sed -n '1,140p'Repository: RocketChat/Rocket.Chat
Length of output: 174
Handle errors for markThreadRead fire-and-forget in ThreadChat.
markThreadRead comes from useEndpoint('POST', '/v1/subscriptions.read') (Promise-returning) and is currently dropped without a rejection handler; the same call in useThreadMessagesQuery.ts already uses .catch(() => undefined). Align ThreadChat.tsx to avoid potential unhandled promise rejections.
Proposed fix
- void markThreadRead({ rid: room._id, tmid: mainMessage._id });
+ void markThreadRead({ rid: room._id, tmid: mainMessage._id }).catch(() => undefined);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void markThreadRead({ rid: room._id, tmid: mainMessage._id }); | |
| void markThreadRead({ rid: room._id, tmid: mainMessage._id }).catch(() => undefined); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsx`
at line 74, The call to markThreadRead in ThreadChat.tsx is fire-and-forget and
can produce an unhandled rejection; update the invocation (the void
markThreadRead({ rid: room._id, tmid: mainMessage._id }) call) to attach a
rejection handler (e.g. .catch(() => undefined) or log the error) so promise
rejections are swallowed/handled consistently with useThreadMessagesQuery.ts;
locate markThreadRead (from useEndpoint('POST', '/v1/subscriptions.read')) in
ThreadChat and modify the call to add the .catch handler.
Comment on lines
+82
to
+87
| const parseSpotlightType = (raw?: string): SpotlightType | undefined => { | ||
| if (!raw) return undefined; | ||
| try { | ||
| const parsed = JSON.parse(raw) as SpotlightType; | ||
| return parsed && typeof parsed === 'object' ? parsed : undefined; | ||
| } catch { |
Contributor
There was a problem hiding this comment.
Tighten type parser to reject arrays/non-plain objects.
parseSpotlightType currently accepts [] because typeof [] === 'object'. On Line 86, validate plain-object shape before returning.
Suggested fix
const parseSpotlightType = (raw?: string): SpotlightType | undefined => {
if (!raw) return undefined;
try {
- const parsed = JSON.parse(raw) as SpotlightType;
- return parsed && typeof parsed === 'object' ? parsed : undefined;
+ const parsed = JSON.parse(raw);
+ if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) {
+ return undefined;
+ }
+ return parsed as SpotlightType;
} catch {
return undefined;
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const parseSpotlightType = (raw?: string): SpotlightType | undefined => { | |
| if (!raw) return undefined; | |
| try { | |
| const parsed = JSON.parse(raw) as SpotlightType; | |
| return parsed && typeof parsed === 'object' ? parsed : undefined; | |
| } catch { | |
| const parseSpotlightType = (raw?: string): SpotlightType | undefined => { | |
| if (!raw) return undefined; | |
| try { | |
| const parsed = JSON.parse(raw); | |
| if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { | |
| return undefined; | |
| } | |
| return parsed as SpotlightType; | |
| } catch { | |
| return undefined; | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rest-typings/src/v1/misc.ts` around lines 82 - 87, The parser
parseSpotlightType currently returns any object including arrays; update its
success check to only accept plain objects by rejecting arrays and non-plain
prototypes: after JSON.parse(parsed) assert parsed && typeof parsed === 'object'
&& !Array.isArray(parsed) && (Object.getPrototypeOf(parsed) === Object.prototype
|| parsed.constructor === Object) before returning, otherwise return undefined;
keep the function name parseSpotlightType and the SpotlightType type for
clarity.
Comment on lines
+54
to
+57
| tmid: { | ||
| type: 'string', | ||
| nullable: true, | ||
| }, |
Contributor
There was a problem hiding this comment.
Keep runtime validation aligned with the declared TypeScript contract.
On Line 54 and Line 72, tmid is nullable: true, but the type is tmid?: IMessage['_id'] (no null). This allows payloads the public type does not advertise.
Suggested fix
tmid: {
type: 'string',
- nullable: true,
},Also applies to: 72-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rest-typings/src/v1/subscriptionsEndpoints.ts` around lines 54 - 57,
The runtime schema for the tmid field allows null (tmid: { type: 'string',
nullable: true }) but the TypeScript contract declares tmid as optional with
type IMessage['_id'] (tmid?: IMessage['_id']) which does not include null;
update the runtime validation to match the TS contract by removing nullable:
true for the tmid schema entries (both occurrences) or change the TypeScript
type to include null if null is intended—ensure the schema and the
IMessage['_id'] usage (tmid) are consistent.
Contributor
There was a problem hiding this comment.
7 issues found across 23 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/api/server/v1/chat.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/chat.ts:550">
P2: Reject requests that include both `fileId` and `msgId` to avoid ambiguous delete-target resolution.</violation>
</file>
<file name="apps/meteor/app/lib/server/methods/joinRoom.ts">
<violation number="1" location="apps/meteor/app/lib/server/methods/joinRoom.ts:19">
P2: Unconditional deprecation log contradicts PR intent — `joinRoom` is still called via DDP for non-channel types (private groups, DMs, livechat) where `channels.join` does not apply. This will produce noisy deprecation warnings for legitimate DDP usage that has no REST equivalent yet. Consider either removing the log or making it conditional on room.t === 'c' after the room is fetched.</violation>
</file>
<file name="packages/rest-typings/src/v1/chat.ts">
<violation number="1" location="packages/rest-typings/src/v1/chat.ts:249">
P2: Use `oneOf` (or make the branches mutually exclusive) so mixed `fileId` + `msgId`/`roomId` payloads are rejected instead of silently taking the file-id branch.
(Based on your team's feedback about keeping API typings aligned with runtime endpoints.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/meteor/client/hooks/useJoinRoom.ts">
<violation number="1" location="apps/meteor/client/hooks/useJoinRoom.ts:22">
P1: Preserve the DDP fallback here. This mutation ignores `type` and always posts to `/v1/channels.join`, so the join button now fails for room types outside the REST route's accepted set.</violation>
</file>
<file name="apps/meteor/client/views/root/hooks/useLoadMissedMessages.ts">
<violation number="1" location="apps/meteor/client/views/root/hooks/useLoadMissedMessages.ts:29">
P1: Handle `result.deleted` as well as `result.updated`; otherwise offline deletions are never reflected in the client store.</violation>
</file>
<file name="apps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsx">
<violation number="1" location="apps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsx:74">
P3: Add `.catch(() => undefined)` to suppress potential unhandled promise rejections—this matches the pattern already used for the same `markThreadRead` call in `useThreadMessagesQuery.ts`.</violation>
</file>
<file name="packages/rest-typings/src/v1/misc.ts">
<violation number="1" location="packages/rest-typings/src/v1/misc.ts:86">
P3: `typeof [] === 'object'` in JavaScript, so this check would accept an array as a valid `SpotlightType`. Add `Array.isArray(parsed)` guard to reject non-plain-object inputs.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Comment on lines
+550
to
553
| 'fileId' in this.bodyParams | ||
| ? await Messages.getMessageByFileId(this.bodyParams.fileId) | ||
| : await Messages.findOneById(this.bodyParams.msgId, { projection: { u: 1, rid: 1 } }); | ||
|
|
Contributor
There was a problem hiding this comment.
P2: Reject requests that include both fileId and msgId to avoid ambiguous delete-target resolution.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/v1/chat.ts, line 550:
<comment>Reject requests that include both `fileId` and `msgId` to avoid ambiguous delete-target resolution.</comment>
<file context>
@@ -546,13 +546,17 @@ const chatEndpoints = API.v1
async function action() {
- const msg = await Messages.findOneById(this.bodyParams.msgId, { projection: { u: 1, rid: 1 } });
+ const msg =
+ 'fileId' in this.bodyParams
+ ? await Messages.getMessageByFileId(this.bodyParams.fileId)
+ : await Messages.findOneById(this.bodyParams.msgId, { projection: { u: 1, rid: 1 } });
</file context>
Suggested change
| 'fileId' in this.bodyParams | |
| ? await Messages.getMessageByFileId(this.bodyParams.fileId) | |
| : await Messages.findOneById(this.bodyParams.msgId, { projection: { u: 1, rid: 1 } }); | |
| if ('fileId' in this.bodyParams && 'msgId' in this.bodyParams) { | |
| return API.v1.failure('Provide either "fileId" or "msgId"/"roomId", not both.'); | |
| } | |
| const msg = | |
| 'fileId' in this.bodyParams | |
| ? await Messages.getMessageByFileId(this.bodyParams.fileId) | |
| : await Messages.findOneById(this.bodyParams.msgId, { projection: { u: 1, rid: 1 } }); |
|
|
||
| Meteor.methods<ServerMethods>({ | ||
| async joinRoom(rid, code) { | ||
| methodDeprecationLogger.method('joinRoom', '9.0.0', '/v1/channels.join'); |
Contributor
There was a problem hiding this comment.
P2: Unconditional deprecation log contradicts PR intent — joinRoom is still called via DDP for non-channel types (private groups, DMs, livechat) where channels.join does not apply. This will produce noisy deprecation warnings for legitimate DDP usage that has no REST equivalent yet. Consider either removing the log or making it conditional on room.t === 'c' after the room is fetched.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/lib/server/methods/joinRoom.ts, line 19:
<comment>Unconditional deprecation log contradicts PR intent — `joinRoom` is still called via DDP for non-channel types (private groups, DMs, livechat) where `channels.join` does not apply. This will produce noisy deprecation warnings for legitimate DDP usage that has no REST equivalent yet. Consider either removing the log or making it conditional on room.t === 'c' after the room is fetched.</comment>
<file context>
@@ -14,6 +16,7 @@ declare module '@rocket.chat/ddp-client' {
Meteor.methods<ServerMethods>({
async joinRoom(rid, code) {
+ methodDeprecationLogger.method('joinRoom', '9.0.0', '/v1/channels.join');
check(rid, String);
</file context>
| }, | ||
| }, | ||
| required: ['msgId', 'roomId'], | ||
| anyOf: [{ required: ['msgId', 'roomId'] }, { required: ['fileId'] }], |
Contributor
There was a problem hiding this comment.
P2: Use oneOf (or make the branches mutually exclusive) so mixed fileId + msgId/roomId payloads are rejected instead of silently taking the file-id branch.
(Based on your team's feedback about keeping API typings aligned with runtime endpoints.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/rest-typings/src/v1/chat.ts, line 249:
<comment>Use `oneOf` (or make the branches mutually exclusive) so mixed `fileId` + `msgId`/`roomId` payloads are rejected instead of silently taking the file-id branch.
(Based on your team's feedback about keeping API typings aligned with runtime endpoints.) </comment>
<file context>
@@ -233,12 +238,15 @@ const ChatDeleteSchema = {
},
},
- required: ['msgId', 'roomId'],
+ anyOf: [{ required: ['msgId', 'roomId'] }, { required: ['fileId'] }],
additionalProperties: false,
};
</file context>
Suggested change
| anyOf: [{ required: ['msgId', 'roomId'] }, { required: ['fileId'] }], | |
| oneOf: [{ required: ['msgId', 'roomId'] }, { required: ['fileId'] }], |
| } | ||
|
|
||
| readThreads(mainMessage._id); | ||
| void markThreadRead({ rid: room._id, tmid: mainMessage._id }); |
Contributor
There was a problem hiding this comment.
P3: Add .catch(() => undefined) to suppress potential unhandled promise rejections—this matches the pattern already used for the same markThreadRead call in useThreadMessagesQuery.ts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsx, line 74:
<comment>Add `.catch(() => undefined)` to suppress potential unhandled promise rejections—this matches the pattern already used for the same `markThreadRead` call in `useThreadMessagesQuery.ts`.</comment>
<file context>
@@ -71,7 +71,7 @@ const ThreadChat = ({ mainMessage }: ThreadChatProps) => {
}
- readThreads(mainMessage._id);
+ void markThreadRead({ rid: room._id, tmid: mainMessage._id });
},
clientCallbacks.priority.MEDIUM,
</file context>
Suggested change
| void markThreadRead({ rid: room._id, tmid: mainMessage._id }); | |
| void markThreadRead({ rid: room._id, tmid: mainMessage._id }).catch(() => undefined); |
| if (!raw) return undefined; | ||
| try { | ||
| const parsed = JSON.parse(raw) as SpotlightType; | ||
| return parsed && typeof parsed === 'object' ? parsed : undefined; |
Contributor
There was a problem hiding this comment.
P3: typeof [] === 'object' in JavaScript, so this check would accept an array as a valid SpotlightType. Add Array.isArray(parsed) guard to reject non-plain-object inputs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/rest-typings/src/v1/misc.ts, line 86:
<comment>`typeof [] === 'object'` in JavaScript, so this check would accept an array as a valid `SpotlightType`. Add `Array.isArray(parsed)` guard to reject non-plain-object inputs.</comment>
<file context>
@@ -38,21 +38,60 @@ const ShieldSvgSchema = {
+ if (!raw) return undefined;
+ try {
+ const parsed = JSON.parse(raw) as SpotlightType;
+ return parsed && typeof parsed === 'object' ? parsed : undefined;
+ } catch {
+ return undefined;
</file context>
Suggested change
| return parsed && typeof parsed === 'object' ? parsed : undefined; | |
| if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { | |
| return undefined; | |
| } | |
| return parsed as SpotlightType; |
ggazzo
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/core-typings/src/IUser.ts (1)
236-238: IUser:providerIdremoval looks safe for in-repo runtime code (no user-providerId usages found).
packages/core-typings/src/IUser.tsno longer definesproviderId.- Repo searches for
IUser/user-document access viaproviderId(e.g.,user/IUser.providerId,['providerId'], and Mongo ops onusers) returned no matches; remainingproviderIdhits are for outbound provider metadata (e.g.,apps/meteor/client/lib/queryKeys.ts).- Remaining risk is limited to external consumers of the typings and any historical persisted user documents that may still contain
providerId—ensure nothing still reads it (migration/cleanup only if needed).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core-typings/src/IUser.ts` around lines 236 - 238, IUser no longer declares providerId which may break external consumers or code reading historical persisted user docs; add a clear JSDoc/deprecation comment in the IUser declaration explaining that providerId was intentionally removed, note the expected migration/cleanup for persisted user documents, and document this breaking change in the package changelog/release notes so consumers know to migrate or re-add an optional providerId if needed (refer to the IUser type and the providerId field).apps/meteor/server/lib/oauth/updateOAuthServices.ts (1)
71-96: ⚡ Quick winRe-check
CustomOAuthinstantiation on settings updates
- Although
updateOAuthServicescallsnew CustomOAuth(serviceKey, ...)each time,CustomOAuthuses a module-levelServicesmap: ifServices[name]already exists, the constructor only runsServices[name].configure(options)and returns before re-registering service/hooks.- Optional: if you want to reduce per-update allocations and make intent clearer, add a dedicated
configure/factory path (or cache the instance) so callers don’t construct throwaway objects.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/server/lib/oauth/updateOAuthServices.ts` around lines 71 - 96, The current updateOAuthServices flow repeatedly calls new CustomOAuth(serviceKey, ...) even though CustomOAuth's constructor checks the module-level Services[name] and simply calls Services[name].configure(options) if already present; change updateOAuthServices to call a dedicated configure path instead of allocating throwaway objects: add or use an exported configure(name, options) function (or a factory like CustomOAuth.configure/serviceConfigure) that looks up Services[serviceKey] and calls .configure(options) or creates and registers a new instance if missing; update the call site (updateOAuthServices) to invoke that configure function with serviceKey and the options object rather than instantiating CustomOAuth with new each update to make intent explicit and avoid unnecessary allocations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/app/apple/lib/handleIdentityToken.ts`:
- Around line 46-50: handleIdentityToken.ts currently sets serviceData.name = ''
which can overwrite an existing user display name on subsequent logins; modify
the flow so an empty name is not persisted — either omit the name property from
the returned identity payload or set it to undefined instead of an empty string,
and ensure AppleCustomOAuth.ts only assigns serviceData.name when the
authorization response contains usrObj.name (firstName/lastName) so you never
send an explicit empty name down to custom_oauth_server.js where it would
overwrite an existing display name.
In `@apps/meteor/app/apple/server/appleOauthRegisterService.ts`:
- Around line 50-61: Before calling KJUR.jws.JWS.sign, guard against an empty or
invalid ES256 private key by validating serverSecret and handling signing
errors: check serverSecret is a non-empty string and optionally verify it
matches expected PEM/JWK/EC key pattern, then wrap the KJUR.jws.JWS.sign call
(the block that produces secret using HEADER, iss, iat, exp, aud, sub,
serverSecret) in a try/catch; on failure log a clear admin-facing error
(including the exception message) and throw or return a controlled error instead
of letting the exception bubble uncaught.
In `@apps/meteor/app/apple/server/loginHandler.ts`:
- Around line 32-35: The call to
Accounts.updateOrCreateUserFromExternalService('apple', serviceData, { profile
}) returns a Promise, so add await before this call to get the resolved result
and then check result.userId; ensure the enclosing function (the Apple login
handler) is declared async if it isn't already so awaiting is valid, and keep
existing error handling that throws "User creation failed" when the awaited
result is undefined or has no userId.
---
Nitpick comments:
In `@apps/meteor/server/lib/oauth/updateOAuthServices.ts`:
- Around line 71-96: The current updateOAuthServices flow repeatedly calls new
CustomOAuth(serviceKey, ...) even though CustomOAuth's constructor checks the
module-level Services[name] and simply calls Services[name].configure(options)
if already present; change updateOAuthServices to call a dedicated configure
path instead of allocating throwaway objects: add or use an exported
configure(name, options) function (or a factory like
CustomOAuth.configure/serviceConfigure) that looks up Services[serviceKey] and
calls .configure(options) or creates and registers a new instance if missing;
update the call site (updateOAuthServices) to invoke that configure function
with serviceKey and the options object rather than instantiating CustomOAuth
with new each update to make intent explicit and avoid unnecessary allocations.
In `@packages/core-typings/src/IUser.ts`:
- Around line 236-238: IUser no longer declares providerId which may break
external consumers or code reading historical persisted user docs; add a clear
JSDoc/deprecation comment in the IUser declaration explaining that providerId
was intentionally removed, note the expected migration/cleanup for persisted
user documents, and document this breaking change in the package
changelog/release notes so consumers know to migrate or re-add an optional
providerId if needed (refer to the IUser type and the providerId field).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da294d22-7d16-42bb-81dc-99bf5593e105
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (76)
.changeset/flat-poets-cheat.mdapps/meteor/app/2fa/server/code/EmailCheck.tsapps/meteor/app/2fa/server/code/EmailCheckForOAuth.tsapps/meteor/app/2fa/server/code/TOTPCheck.tsapps/meteor/app/2fa/server/code/TOTPCheckForOAuth.tsapps/meteor/app/2fa/server/code/index.tsapps/meteor/app/api/server/ApiClass.tsapps/meteor/app/api/server/index.tsapps/meteor/app/api/server/v1/twoFactorChallenges.tsapps/meteor/app/apple/lib/handleIdentityToken.tsapps/meteor/app/apple/server/appleOauthRegisterService.tsapps/meteor/app/apple/server/loginHandler.tsapps/meteor/app/custom-oauth/server/customOAuth.tsapps/meteor/app/custom-oauth/server/custom_oauth_server.jsapps/meteor/app/custom-sounds/server/methods/listCustomSounds.tsapps/meteor/app/dolphin/server/lib.tsapps/meteor/app/drupal/server/lib.tsapps/meteor/app/gitlab/server/lib.tsapps/meteor/app/lib/server/methods/createToken.tsapps/meteor/app/linkedin/server/index.tsapps/meteor/app/linkedin/server/lib.tsapps/meteor/app/meteor-developer/server/index.tsapps/meteor/app/meteor-developer/server/lib.tsapps/meteor/app/nextcloud/server/lib.tsapps/meteor/app/wordpress/server/lib.tsapps/meteor/client/components/TwoFactorModal/TwoFactorEmailModal.tsxapps/meteor/client/components/TwoFactorModal/TwoFactorModal.tsxapps/meteor/client/lib/2fa/process2faReturn.tsapps/meteor/client/lib/buildAuthDeeplinkURL.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/client/providers/CustomSoundProvider/CustomSoundProvider.tsxapps/meteor/client/providers/CustomSoundProvider/lib/helpers.tsapps/meteor/client/startup/routes.tsxapps/meteor/client/views/OAuthTwoFactorAuthentication/OAuthTwoFactorAuthenticationRouter.tsxapps/meteor/client/views/root/AppLayout.tsxapps/meteor/client/views/root/hooks/useLoginOtherClients.tsapps/meteor/client/views/root/hooks/useLoginViaQuery.tsapps/meteor/client/views/root/hooks/useShareSessionWithOtherClients.tsapps/meteor/definition/externals/express-session.d.tsapps/meteor/definition/externals/express.d.tsapps/meteor/definition/externals/meteor/accounts-base.d.tsapps/meteor/package.jsonapps/meteor/server/configuration/accounts_meld.jsapps/meteor/server/configuration/configurePassport.tsapps/meteor/server/configuration/index.tsapps/meteor/server/importPackages.tsapps/meteor/server/lib/oauth/addPassportCustomOAuth.tsapps/meteor/server/lib/oauth/configureOAuthServices.tsapps/meteor/server/lib/oauth/createOAuthServiceConfig.tsapps/meteor/server/lib/oauth/getOAuthServices.tsapps/meteor/server/lib/oauth/oauthConfigs.tsapps/meteor/server/lib/oauth/passportOAuthCallback.tsapps/meteor/server/lib/oauth/twoFactorAuth.tsapps/meteor/server/lib/oauth/updateOAuthServices.tsapps/meteor/server/lib/oauth/verifyFunction.tsapps/meteor/server/models.tsapps/meteor/server/settings/oauth.tsapps/meteor/tests/e2e/fixtures/addCustomOAuth.tspackages/core-typings/src/ILoginServiceConfiguration.tspackages/core-typings/src/ITwoFactorChallenge.tspackages/core-typings/src/IUser.tspackages/core-typings/src/index.tspackages/desktop-api/src/index.tspackages/i18n/src/locales/en.i18n.jsonpackages/model-typings/src/index.tspackages/model-typings/src/models/ITwoFactorChallengesModel.tspackages/models/src/index.tspackages/models/src/modelClasses.tspackages/models/src/models/TwoFactorChallenges.tspackages/rest-typings/src/index.tspackages/rest-typings/src/v1/twoFactorChallenges.tspackages/web-ui-registration/global.d.tspackages/web-ui-registration/src/LoginServices.tsxpackages/web-ui-registration/src/LoginServicesButton.tsxpackages/web-ui-registration/tsconfig.build.jsonpackages/web-ui-registration/tsconfig.json
💤 Files with no reviewable changes (46)
- apps/meteor/client/lib/buildAuthDeeplinkURL.ts
- apps/meteor/app/2fa/server/code/EmailCheckForOAuth.ts
- .changeset/flat-poets-cheat.md
- apps/meteor/definition/externals/express-session.d.ts
- apps/meteor/app/2fa/server/code/TOTPCheckForOAuth.ts
- packages/web-ui-registration/global.d.ts
- apps/meteor/client/views/root/hooks/useShareSessionWithOtherClients.ts
- apps/meteor/app/custom-oauth/server/custom_oauth_server.js
- apps/meteor/server/lib/oauth/getOAuthServices.ts
- apps/meteor/server/lib/oauth/verifyFunction.ts
- apps/meteor/app/linkedin/server/index.ts
- packages/rest-typings/src/v1/twoFactorChallenges.ts
- packages/models/src/modelClasses.ts
- apps/meteor/app/meteor-developer/server/lib.ts
- apps/meteor/app/linkedin/server/lib.ts
- apps/meteor/client/views/root/hooks/useLoginOtherClients.ts
- packages/core-typings/src/ITwoFactorChallenge.ts
- apps/meteor/app/api/server/v1/twoFactorChallenges.ts
- apps/meteor/server/lib/oauth/oauthConfigs.ts
- packages/models/src/models/TwoFactorChallenges.ts
- apps/meteor/tests/e2e/fixtures/addCustomOAuth.ts
- apps/meteor/server/lib/oauth/passportOAuthCallback.ts
- apps/meteor/server/lib/oauth/twoFactorAuth.ts
- apps/meteor/server/configuration/configurePassport.ts
- apps/meteor/server/lib/oauth/createOAuthServiceConfig.ts
- apps/meteor/client/views/OAuthTwoFactorAuthentication/OAuthTwoFactorAuthenticationRouter.tsx
- apps/meteor/server/lib/oauth/addPassportCustomOAuth.ts
- packages/model-typings/src/index.ts
- apps/meteor/server/settings/oauth.ts
- apps/meteor/definition/externals/express.d.ts
- apps/meteor/server/configuration/index.ts
- packages/core-typings/src/ILoginServiceConfiguration.ts
- apps/meteor/client/startup/routes.tsx
- packages/model-typings/src/models/ITwoFactorChallengesModel.ts
- packages/rest-typings/src/index.ts
- apps/meteor/app/meteor-developer/server/index.ts
- apps/meteor/client/views/root/AppLayout.tsx
- apps/meteor/server/lib/oauth/configureOAuthServices.ts
- packages/core-typings/src/index.ts
- packages/models/src/index.ts
- packages/desktop-api/src/index.ts
- apps/meteor/server/models.ts
- apps/meteor/app/custom-oauth/server/customOAuth.ts
- packages/i18n/src/locales/en.i18n.json
- apps/meteor/package.json
- apps/meteor/server/importPackages.ts
✅ Files skipped from review due to trivial changes (3)
- packages/web-ui-registration/tsconfig.json
- apps/meteor/app/2fa/server/code/TOTPCheck.ts
- packages/web-ui-registration/tsconfig.build.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/apple/server/loginHandler.tsapps/meteor/client/components/TwoFactorModal/TwoFactorModal.tsxapps/meteor/app/custom-sounds/server/methods/listCustomSounds.tsapps/meteor/client/providers/CustomSoundProvider/lib/helpers.tsapps/meteor/client/views/root/hooks/useLoginViaQuery.tspackages/core-typings/src/IUser.tsapps/meteor/app/drupal/server/lib.tsapps/meteor/app/dolphin/server/lib.tsapps/meteor/app/lib/server/methods/createToken.tsapps/meteor/client/providers/CustomSoundProvider/CustomSoundProvider.tsxapps/meteor/app/api/server/ApiClass.tsapps/meteor/app/2fa/server/code/EmailCheck.tsapps/meteor/definition/externals/meteor/accounts-base.d.tsapps/meteor/app/gitlab/server/lib.tspackages/web-ui-registration/src/LoginServicesButton.tsxapps/meteor/client/components/TwoFactorModal/TwoFactorEmailModal.tsxapps/meteor/app/wordpress/server/lib.tsapps/meteor/app/api/server/index.tspackages/web-ui-registration/src/LoginServices.tsxapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/server/lib/oauth/updateOAuthServices.tsapps/meteor/app/nextcloud/server/lib.tsapps/meteor/app/apple/lib/handleIdentityToken.tsapps/meteor/app/apple/server/appleOauthRegisterService.tsapps/meteor/server/configuration/accounts_meld.jsapps/meteor/app/2fa/server/code/index.tsapps/meteor/client/lib/2fa/process2faReturn.ts
🧠 Learnings (6)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/apple/server/loginHandler.tsapps/meteor/app/custom-sounds/server/methods/listCustomSounds.tsapps/meteor/client/providers/CustomSoundProvider/lib/helpers.tsapps/meteor/client/views/root/hooks/useLoginViaQuery.tspackages/core-typings/src/IUser.tsapps/meteor/app/drupal/server/lib.tsapps/meteor/app/dolphin/server/lib.tsapps/meteor/app/lib/server/methods/createToken.tsapps/meteor/app/api/server/ApiClass.tsapps/meteor/app/2fa/server/code/EmailCheck.tsapps/meteor/definition/externals/meteor/accounts-base.d.tsapps/meteor/app/gitlab/server/lib.tsapps/meteor/app/wordpress/server/lib.tsapps/meteor/app/api/server/index.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/server/lib/oauth/updateOAuthServices.tsapps/meteor/app/nextcloud/server/lib.tsapps/meteor/app/apple/lib/handleIdentityToken.tsapps/meteor/app/apple/server/appleOauthRegisterService.tsapps/meteor/app/2fa/server/code/index.tsapps/meteor/client/lib/2fa/process2faReturn.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/apple/server/loginHandler.tsapps/meteor/app/custom-sounds/server/methods/listCustomSounds.tsapps/meteor/client/providers/CustomSoundProvider/lib/helpers.tsapps/meteor/client/views/root/hooks/useLoginViaQuery.tspackages/core-typings/src/IUser.tsapps/meteor/app/drupal/server/lib.tsapps/meteor/app/dolphin/server/lib.tsapps/meteor/app/lib/server/methods/createToken.tsapps/meteor/app/api/server/ApiClass.tsapps/meteor/app/2fa/server/code/EmailCheck.tsapps/meteor/definition/externals/meteor/accounts-base.d.tsapps/meteor/app/gitlab/server/lib.tsapps/meteor/app/wordpress/server/lib.tsapps/meteor/app/api/server/index.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/server/lib/oauth/updateOAuthServices.tsapps/meteor/app/nextcloud/server/lib.tsapps/meteor/app/apple/lib/handleIdentityToken.tsapps/meteor/app/apple/server/appleOauthRegisterService.tsapps/meteor/app/2fa/server/code/index.tsapps/meteor/client/lib/2fa/process2faReturn.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/app/apple/server/loginHandler.tsapps/meteor/client/components/TwoFactorModal/TwoFactorModal.tsxapps/meteor/app/custom-sounds/server/methods/listCustomSounds.tsapps/meteor/client/providers/CustomSoundProvider/lib/helpers.tsapps/meteor/client/views/root/hooks/useLoginViaQuery.tspackages/core-typings/src/IUser.tsapps/meteor/app/drupal/server/lib.tsapps/meteor/app/dolphin/server/lib.tsapps/meteor/app/lib/server/methods/createToken.tsapps/meteor/client/providers/CustomSoundProvider/CustomSoundProvider.tsxapps/meteor/app/api/server/ApiClass.tsapps/meteor/app/2fa/server/code/EmailCheck.tsapps/meteor/definition/externals/meteor/accounts-base.d.tsapps/meteor/app/gitlab/server/lib.tspackages/web-ui-registration/src/LoginServicesButton.tsxapps/meteor/client/components/TwoFactorModal/TwoFactorEmailModal.tsxapps/meteor/app/wordpress/server/lib.tsapps/meteor/app/api/server/index.tspackages/web-ui-registration/src/LoginServices.tsxapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/server/lib/oauth/updateOAuthServices.tsapps/meteor/app/nextcloud/server/lib.tsapps/meteor/app/apple/lib/handleIdentityToken.tsapps/meteor/app/apple/server/appleOauthRegisterService.tsapps/meteor/app/2fa/server/code/index.tsapps/meteor/client/lib/2fa/process2faReturn.ts
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/components/TwoFactorModal/TwoFactorModal.tsxapps/meteor/client/providers/CustomSoundProvider/CustomSoundProvider.tsxpackages/web-ui-registration/src/LoginServicesButton.tsxapps/meteor/client/components/TwoFactorModal/TwoFactorEmailModal.tsxpackages/web-ui-registration/src/LoginServices.tsx
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/providers/CustomSoundProvider/lib/helpers.tsapps/meteor/client/views/root/hooks/useLoginViaQuery.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/client/lib/2fa/process2faReturn.ts
📚 Learning: 2026-05-11T20:30:35.265Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 40480
File: apps/meteor/client/meteor/startup/accounts.ts:59-61
Timestamp: 2026-05-11T20:30:35.265Z
Learning: In Rocket.Chat’s Meteor client code, when calling `dispatchToastMessage` with `{ type: 'error' }`, pass the raw caught error object as `message` without manual normalization. `dispatchToastMessage` is designed to accept `message: unknown` for error toasts, so avoid converting errors to strings (e.g., `String(error)`) or extracting `error.message` before passing them.
Applied to files:
apps/meteor/client/providers/CustomSoundProvider/lib/helpers.tsapps/meteor/client/views/root/hooks/useLoginViaQuery.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/client/lib/2fa/process2faReturn.ts
🔇 Additional comments (29)
apps/meteor/client/components/TwoFactorModal/TwoFactorModal.tsx (1)
24-36: LGTM!apps/meteor/app/custom-sounds/server/methods/listCustomSounds.ts (1)
6-7: LGTM!Also applies to: 17-17
apps/meteor/client/providers/CustomSoundProvider/lib/helpers.ts (1)
7-9: LGTM!apps/meteor/client/views/root/hooks/useLoginViaQuery.ts (1)
10-14: LGTM!apps/meteor/app/drupal/server/lib.ts (1)
1-33: LGTM!apps/meteor/app/dolphin/server/lib.ts (1)
1-54: LGTM!apps/meteor/app/lib/server/methods/createToken.ts (1)
19-21: LGTM!apps/meteor/client/providers/CustomSoundProvider/CustomSoundProvider.tsx (1)
3-3: LGTM!Also applies to: 19-19, 25-35
apps/meteor/app/2fa/server/code/EmailCheck.ts (2)
13-13: LGTM!
146-149: 💤 Low valueRemove/justify the
as booleancast inEmailCheck.ts.
maxFaildedAttemtpsReachedcastsawait Users.maxInvalidEmailCodeAttemptsReached(...)toboolean; if the model already returnsPromise<boolean>, the cast is redundant. If not, the cast can hide a real type mismatch—confirm the model’s return type and either drop the assertion or fix the model signature.apps/meteor/app/gitlab/server/lib.ts (1)
1-31: LGTM!packages/web-ui-registration/src/LoginServicesButton.tsx (1)
30-37: LGTM!apps/meteor/client/components/TwoFactorModal/TwoFactorEmailModal.tsx (1)
4-4: VerifyTwoFactorModalwiring foremailOrUsernameprop
TwoFactorEmailModalnow requiresemailOrUsername: string(and theresendEmailcallback prop is removed). Ensureapps/meteor/client/components/TwoFactorModal/TwoFactorModal.tsxpassesemailOrUsernametoTwoFactorEmailModalfor the email flow (props.method === Method.EMAIL) so the updated props contract is satisfied.apps/meteor/app/api/server/ApiClass.ts (1)
147-161: VerifygenerateConnectionhas no external consumers
Search for thegenerateConnectionidentifier in other TypeScript files returned no matches, so it appears to be internal-only.apps/meteor/definition/externals/meteor/accounts-base.d.ts (1)
26-26: Confirmaccounts-base.d.tsreturn types match Meteor’s implementation.
_insertLoginToken: Meteor awaits it for side effects and the resolved value isn’t used in the documented flow, sovoidis consistent.updateOrCreateUserFromExternalService: the Meteor docs/source we found describe the hook contract (setAdditionalFindUserOnExternalLoginreturns a user orundefined), but don’t specify the exact return type ofupdateOrCreateUserFromExternalServiceitself—re-check against the specific Meteor version before assuming the Promise→sync change (and removal of| undefined) is safe.apps/meteor/app/wordpress/server/lib.ts (1)
1-95: LGTM!apps/meteor/app/api/server/index.ts (1)
48-50: LGTM!packages/web-ui-registration/src/LoginServices.tsx (1)
1-40: LGTM!apps/meteor/app/nextcloud/server/lib.ts (1)
1-38: LGTM!apps/meteor/client/lib/sdk/ddpSdk.ts (1)
71-71: ConfirmreadStoredLoginTokenhas no external consumers (module-local).
A repo-wide search showsreadStoredLoginTokenis only defined and referenced withinapps/meteor/client/lib/sdk/ddpSdk.ts(definition at line 71; internal uses at lines 94 and 131). No other modules import or referencereadStoredLoginTokenfromddpSdk.ts(other imports from the file are forgetDdpSdk/ensureConnectedAndAuthenticated).apps/meteor/server/configuration/accounts_meld.js (1)
21-24: LGTM!apps/meteor/app/2fa/server/code/index.ts (4)
66-77: LGTM!
121-139: LGTM!
149-165: LGTM!
167-240: LGTM!apps/meteor/client/lib/2fa/process2faReturn.ts (4)
31-38: LGTM!
40-84: LGTM!
86-121: LGTM!
123-167: Email 2FA “resend code” remains implemented (noresendEmailprop required)
invokeTwoFactorModalpassesemailOrUsernamethrough toTwoFactorModalvia...props, andTwoFactorEmailModalitself renders the “Cloud_resend_email” button and performs the resend by callingPOST /v1/users.2fa.sendEmailCodeinonClickResendCode(so removing aresendEmailprop from the modal props won’t break resend).
ggazzo
force-pushed
the
chore/ddp-migrate-batch2
branch
from
4deb36d to
7dbdade
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/ddp-migrate-batch2-callers.md:
- Around line 5-9: Update the change note so it accurately describes that only
the channel-specific joinRoom flow was migrated: clarify that joinRoom for
room.type === 'c' now maps to POST /v1/channels.join and that non-channel
joinRoom paths remain on DDP and are not deprecation-logged; reference the
joinRoom DDP handler and the REST route POST /v1/channels.join in the wording to
make the distinction explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85b2485b-c201-406d-8bd7-015e3d632c98
📒 Files selected for processing (5)
.changeset/ddp-migrate-batch2-callers.md.changeset/rest-chat-delete-by-fileid.md.changeset/rest-spotlight-params-and-anonymous.md.changeset/rest-subscriptions-read-tmid.md.changeset/rest-users-setpreferences-utcoffset.md
✅ Files skipped from review due to trivial changes (4)
- .changeset/rest-users-setpreferences-utcoffset.md
- .changeset/rest-chat-delete-by-fileid.md
- .changeset/rest-subscriptions-read-tmid.md
- .changeset/rest-spotlight-params-and-anonymous.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 📦 Meteor Build (coverage)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.
Applied to files:
.changeset/ddp-migrate-batch2-callers.md
coderabbitai
Bot
added
type: feature
Contributor
|
Actionable comments posted: 0 |
Contributor
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/api/server/v1/chat.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/chat.ts:550">
P2: Reject requests that include both `fileId` and `msgId` to avoid ambiguous delete-target resolution.</violation>
</file>
<file name="apps/meteor/app/lib/server/methods/joinRoom.ts">
<violation number="1" location="apps/meteor/app/lib/server/methods/joinRoom.ts:19">
P2: Unconditional deprecation log contradicts PR intent — `joinRoom` is still called via DDP for non-channel types (private groups, DMs, livechat) where `channels.join` does not apply. This will produce noisy deprecation warnings for legitimate DDP usage that has no REST equivalent yet. Consider either removing the log or making it conditional on room.t === 'c' after the room is fetched.</violation>
</file>
<file name="packages/rest-typings/src/v1/chat.ts">
<violation number="1" location="packages/rest-typings/src/v1/chat.ts:249">
P2: Use `oneOf` (or make the branches mutually exclusive) so mixed `fileId` + `msgId`/`roomId` payloads are rejected instead of silently taking the file-id branch.
(Based on your team's feedback about keeping API typings aligned with runtime endpoints.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsx">
<violation number="1" location="apps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsx:74">
P3: Add `.catch(() => undefined)` to suppress potential unhandled promise rejections—this matches the pattern already used for the same `markThreadRead` call in `useThreadMessagesQuery.ts`.</violation>
</file>
<file name="packages/rest-typings/src/v1/misc.ts">
<violation number="1" location="packages/rest-typings/src/v1/misc.ts:86">
P3: `typeof [] === 'object'` in JavaScript, so this check would accept an array as a valid `SpotlightType`. Add `Array.isArray(parsed)` guard to reject non-plain-object inputs.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
This was referenced May 28, 2026
Dispatches by room type. For `type === 'c'` the channel case hits the REST endpoint; other types (private groups, DMs, livechat) keep the DDP `joinRoom` method until /v1/* coverage catches up.
… join Drop the DDP fallback — non-channel rooms already failed via DDP too; the REST error path is equivalent. TODO marks the gap until a type-agnostic REST endpoint exists.
userSetUtcOffset → /v1/users.setPreferences (utcOffset field added; server saves at user-document root via Users.setUtcOffset, not the settings.preferences subdoc). deleteFileMessage → /v1/chat.delete (body now accepts fileId as an alternative to msgId+roomId; server resolves the message via Messages.getMessageByFileId). readThreads → /v1/subscriptions.read (body now accepts tmid; server routes to readThread(tmid) instead of readMessages). spotlight → /v1/spotlight (query now accepts usernames CSV, type JSON, rid; response type includes nickname/outside/uids/usernames/fname that the DDP version already returned). Each DDP method gets a deprecation log pointing at the REST route.
Server returns outside/external users without a status field (and DDP callers happily handled the absence). Marking it required broke the response validation: GET /v1/spotlight 'must have required property status'.
The hooks/useJoinRoom was already migrated. data.ts (the chat-data API consumed by message flows) still went through DDP, which throws under TEST_MODE='true' because of the deprecation log and broke beforeEach hooks in anonymous-user / embedded-layout specs.
DDP spotlight ran for anonymous DDP sessions (no this.userId), which was how the anonymous-user e2e specs reached 'general' via the navbar search. The REST migration kept authRequired: true and broke those specs (along with embedded-layout) because navbar.openChat hung on the empty listbox. Set authRequired: false and pass the optional this.userId through.
The REST endpoint already exists (paginated). Caller in
CustomSoundProvider switches from `sdk.call('listCustomSounds')` to
`useEndpoint('GET', '/v1/custom-sounds.list')`, drops `_updatedAt`
from the spread to satisfy the Omit<…, '_updatedAt'> return type, and
the helper now takes a narrower shape so REST + the local
defaultSounds list type-check the same way.
Server method gets a deprecation log pointing at the REST route.
Five entries: one per touched REST endpoint (chat.delete, users.setPreferences, subscriptions.read, spotlight) plus a single summary for the client caller swaps that don't change any REST shape.
- CustomSoundProvider: page through /v1/custom-sounds.list until total reached so all sounds load (legacy listCustomSounds returned all at once) - threads: extract readThreadMethod replicating legacy readThreads behavior (Threads_enabled, room access, beforeReadMessages callback); use it in both the DDP method and the subscriptions.read tmid branch - chat.delete: support deleting an uploaded file with no associated message via the Uploads store; make _id/ts/message optional in the response - useLoadMissedMessages: only upsert genuinely-new or already-loaded synced messages and remove deleted ones, avoiding stale-history pollution from the _updatedAt-based chat.syncMessages - add API tests for subscriptions.read (tmid), chat.delete (fileId), spotlight (type/usernames/anonymous) and users.setPreferences (utcOffset)
ggazzo
force-pushed
the
chore/ddp-migrate-batch2
branch
from
2379bca to
46a920e
Compare
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/api/server/v1/chat.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/chat.ts:550">
P2: Reject requests that include both `fileId` and `msgId` to avoid ambiguous delete-target resolution.</violation>
</file>
<file name="apps/meteor/app/lib/server/methods/joinRoom.ts">
<violation number="1" location="apps/meteor/app/lib/server/methods/joinRoom.ts:19">
P2: Unconditional deprecation log contradicts PR intent — `joinRoom` is still called via DDP for non-channel types (private groups, DMs, livechat) where `channels.join` does not apply. This will produce noisy deprecation warnings for legitimate DDP usage that has no REST equivalent yet. Consider either removing the log or making it conditional on room.t === 'c' after the room is fetched.</violation>
</file>
<file name="packages/rest-typings/src/v1/chat.ts">
<violation number="1" location="packages/rest-typings/src/v1/chat.ts:249">
P2: Use `oneOf` (or make the branches mutually exclusive) so mixed `fileId` + `msgId`/`roomId` payloads are rejected instead of silently taking the file-id branch.
(Based on your team's feedback about keeping API typings aligned with runtime endpoints.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsx">
<violation number="1" location="apps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsx:74">
P3: Add `.catch(() => undefined)` to suppress potential unhandled promise rejections—this matches the pattern already used for the same `markThreadRead` call in `useThreadMessagesQuery.ts`.</violation>
</file>
<file name="packages/rest-typings/src/v1/misc.ts">
<violation number="1" location="packages/rest-typings/src/v1/misc.ts:86">
P3: `typeof [] === 'object'` in JavaScript, so this check would accept an array as a valid `SpotlightType`. Add `Array.isArray(parsed)` guard to reject non-plain-object inputs.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
ggazzo
force-pushed
the
chore/ddp-migrate-batch2
branch
from
8725fba to
797dae4
Compare
- chat.delete: revert error string from 'id:' back to 'id of' (regression) - chat.delete test: orphan file (no associated message) returns 400, not 200 - spotlight test: query a prefix so the usernames exclusion path runs (exact-username matches bypass the exclusion list) - subscriptions.read test: tunread is left as an empty array, not unset
sampaiodiego
previously approved these changes
Jun 12, 2026
The thread read-marker migration (subscriptions.read tmid endpoint + ThreadChat/useThreadMessagesQuery callers) moves to its own PR #40957.
Contributor
There was a problem hiding this comment.
2 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsx">
<violation number="1">
P1: Custom agent: **Detect usage of deprecated HTTP endpoints or API helpers**
Use of deprecated DDP method `readThreads` instead of the migrated REST endpoint `POST /v1/subscriptions.read`</violation>
</file>
<file name="apps/meteor/client/views/room/contextualBar/Threads/hooks/useThreadMessagesQuery.ts">
<violation number="1">
P1: Custom agent: **Detect usage of deprecated HTTP endpoints or API helpers**
Introduces deprecated DDP method 'readThreads' while a REST alternative was already in use, contradicting the PR's DDP→REST migration goal</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
sampaiodiego
approved these changes
Jun 15, 2026
This was referenced Jun 17, 2026
ggazzo
added a commit
that referenced
this pull request
Jun 17, 2026
Reverts the loadMissedMessages deprecation added in #40711. Removes the new chat.history REST endpoint (types, tests) and points the reconnect catch-up back at the loadMissedMessages DDP method. chat.syncMessages would be the right long-term target (it also reconciles edits/deletions on reconnect), but its _updatedAt + trash-collection query is currently too slow for this path; left a TODO to optimize that query before migrating.
ggazzo
added a commit
that referenced
this pull request
Jun 17, 2026
Open
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second batch of the DDP→REST caller migration. Adds three small REST endpoint extensions (so the client can stop hitting DDP for these flows) and swaps six client call-sites over to REST. Each DDP method stays registered on the server with a deprecation log pointing at the REST route — external SDK / mobile clients keep working until the 9.0.0 sweep removes them.
REST endpoint changes
POST /v1/chat.delete{ fileId, asUser? }as an alternative to{ msgId, roomId, asUser? }. WhenfileIdis supplied the server resolves the owning message viaMessages.getMessageByFileId.POST /v1/users.setPreferencesdata.utcOffset(number) is now accepted. Server writes to the user-document root viaUsers.setUtcOffset(not thesettings.preferencessubdoc), matching the legacy DDP behavior.GET /v1/spotlightusernames(CSV),type(JSON-encoded{ users?, mentions?, rooms?, includeFederatedRooms? }) andrid. Response items exposenickname/outside/uids/usernames/fnamethat DDP already returned.statuson user items is optional (outside/federated users never had one).authRequiredflipped tofalseto keepAccounts_AllowAnonymousReadflows working.Caller migrations
loadMissedMessagesGET /v1/chat.syncMessagesuseLoadMissedMessages.ts(maps each result throughmapMessageFromApi)joinRoomPOST /v1/channels.joinhooks/useJoinRoom.ts,lib/chats/data.ts(channel-only; non-crooms error via REST same as DDP)userSetUtcOffsetPOST /v1/users.setPreferencesclient/startup/startup.ts(2 sites)deleteFileMessagePOST /v1/chat.deleteuseDeleteFile.tsxspotlightGET /v1/spotlightuseSearchItems.ts,ComposerPopupProvider.tsxlistCustomSoundsGET /v1/custom-sounds.listCustomSoundProvider.tsxA
TODO(ddp-removal)comment onuseJoinRoomanddata.tsflags the spot where we'd want a type-agnostic/v1/rooms.joinlater.Test plan
user.utcOffsetupdates.@user/#roomfrom the composer.Accounts_AllowAnonymousRead) → navbar search resolvesgeneral.Task: ARCH-2164